postupgrade: Fix script for submodules, merge correctly, keep .git #441

creotutar · 2020-10-28T19:42:01Z

We fix the postupgrade script by doing two things:

(1) Only remove files in the theme folder if it is not a submodule. The
default for the jambo init command is to have theme folders added as
submodules. Before we were removing the .git folder which made it so
that we could not see the history of the theme folder. We can now see
the full theme git history after an upgrade.

(2) Merge the correct global_config files. Before, we were merging the
theme global_config.json pre-upgrade and post-upgrade. That is, we did
not touch the user's global_config.json. This is not the desired
behavior, and instead, we would like to see new fields from any new theme
global_config.json file in the user-defined config/global_config.json.
So we change the files being merged.

Note: This means that on postupgrade, we update the user's
config/global_config.json with any new fields. We do not delete any
fields.

Note: This does not touch legacy upgrades.

Add tests for postupgrade merge JSON to make sure we keep certain
assumptions about this global_config merge.

First, it should always give precedence to the original config.
Second, it should not delete any custom fields added to the
global_config.
Third, it should be completely additive and add new fields if they exist
in any of the configuration parameters.

J=SLAP-578
TEST=manual

npm test

Test on a completely new jambo init site with the HH theme. Try jambo upgrade and make sure there are no errors. The error we don't want to
see is below.

fatal: Path 'themes/answers-hitchhiker-theme/global_config.json' exists on disk, but not in 'HEAD'.
 Error: fatal: Path 'themes/answers-hitchhiker-theme/global_config.json' exists on disk, but not in 'HEAD'.

    at GitExecutorChain.onFatalException (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:60:87)
    at GitExecutorChain.<anonymous> (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:51:28)
    at Generator.throw (<anonymous>)
    at rejected (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:6:65)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
Error occurred on node version: v12.16.1

Used below command to clear the folder

rm -rf ./* && rm -rf .git* && jambo init && jambo import --theme
answers-hitchhiker-theme && npm install && git add -A && git commit -m
"init"

We fix the postupgrade script by doing two things: (1) Only remove files in the theme folder if it is not a submodule. The default for the jambo init command is to have theme folders added as submodules. Before we were removing the .git folder which made it so that we could not see the history of the theme folder. We can now see the full theme git history after an upgrade. (2) Merge the correct global_config files. Before, we were merging the theme global_config.json pre-upgrade and post-upgrade. That is, we did not touch the user's global_config.json. This is not the desired behavior, and instead, we would like to see new fields from any new theme global_config.json file in the user-defined config/global_config.json. So we change the files being merged. Note: This means that on postupgrade, we update the user's config/global_config.json with any new fields. We do not delete any fields. Note: This does not touch legacy upgrades. Add tests for postupgrade merge JSON to make sure we keep certain assumptions about this global_config merge. First, it should always give precedence to the original config. Second, it should not delete any custom fields added to the global_config. Third, it should be completely additive and add new fields if they exist in any of the configuration parameters. J=SLAP-578 TEST=manual npm test Test on a completely new `jambo init` site with the HH theme. Try `jambo upgrade` and make sure there are no errors. The error we don't want to see is below. ``` fatal: Path 'themes/answers-hitchhiker-theme/global_config.json' exists on disk, but not in 'HEAD'. Error: fatal: Path 'themes/answers-hitchhiker-theme/global_config.json' exists on disk, but not in 'HEAD'. at GitExecutorChain.onFatalException (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:60:87) at GitExecutorChain.<anonymous> (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:51:28) at Generator.throw (<anonymous>) at rejected (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:6:65) at processTicksAndRejections (internal/process/task_queues.js:97:5) Error occurred on node version: v12.16.1 ``` Used below command to clear the folder ``` rm -rf ./* && rm -rf .git* && jambo init && jambo import --theme answers-hitchhiker-theme && npm install && git add -A && git commit -m "init" ```

oshi97

lgtm as long as rose was 👍 with the new global config behavior! just a few small nits

oshi97 · 2020-10-28T19:53:24Z

tests/postupgrade/utils.js

+const fs = require('fs');
+
+describe('postupgrade utils', () => {
+  describe('mergeJson', () => {


love the test! does mergeJson need it's own describe block right now? We can always give it it's own later if need be

Hm, I was copying the format of the current formatters.js tests. I do kind of like keeping it in its own describe block because it feels easier to remember to do now than later

Are there any negatives I'm missing?

I just don't think it's needed right now, and don't really foresee adding additional tests for mergeJson (it's only 2 lines long). totally up to you!

postupgrade/PostUpgradeHandler.js

oshi97 · 2020-10-28T19:56:57Z

tests/postupgrade/utils.js

+    it('puts together two comment JSON strings', () => {
+      const config1 = fs.readFileSync('./tests/postupgrade/config1.json', 'utf-8');
+      const config2 = fs.readFileSync('./tests/postupgrade/config2.json', 'utf-8');
+      const expected = fs.readFileSync('./tests/postupgrade/config3.json', 'utf-8').trim();


is the trim() necessary?

tests/postupgrade/utils.js

postupgrade/PostUpgradeHandler.js

creotutar · 2020-10-29T03:26:41Z

Yes yes! Had a meeting with @rosiegrant this morning to discuss

## Version 1.17.0 Version 1.17.0 of the Theme supports the new Overlay integration. It also has a number of changes to support the new, built-in Product entity type. ### Changes - A new file, static/js/formatters-custom.js was added to the Theme. This file is where any custom formatters or overrides of built-in formatters, should live. (#448) - Single language cards now use the `toLocalizedDistance` formatter to properly format entity distance. (#449) - To better support WCAG, the iFrame integration now has a `title` attribute on the iFrame. (#451) - The Theme now allows a new type of Answers integration: the Overlay. This is one of the simplest integrations offered. Additional documentation on it can be found in the Hitchhiker training. - We upgraded the Theme to use Webpack v5. This will enable proper tree-shaking of bundle.js. (#463, #470) - A new card type was added: `product-prominentimage-clickable`. The card itself is clickable. (#466) - A new formatter was added: `Formatter.hoursList`. This formatter lists the hours for an entity for every day of the week. (#467) - To improve site performance, the `defer` attribute was added to all the SDK JS assets. (#473, #474) - Small updates were made to all the product cards to support the new, built-in Product entity type. (#411, #476) - A new formatter was added for prices: `Formatter.price`. (#472) - The `JAMBO_INJECTED_DATA` environment variable now includes the label of an experience's vertical. This label is used as the default vertical name in navigation, no results, and universal section title. (#490, 493) ### Bug Fixes - The post upgrade script now works correctly when the Theme has been imported as a submodule. (#441) - The `LocationStandard` card's styling was updated to be fully compatible with IE11. (#455) - A bug was fixed that caused the Theme's inline Javascript to be minified twice. By removing this bug, Webpack builds were made considerably more performant. (#461) - Fixes were made to ensure the `View Results` button for `CollapsibleFilters` is sticky. (#468, #469) - The logic for identifying absolute URLs in the HBS templates was corrected (#475) - The thumbnail selection logic in `Formatter.image` was incorrect. It was not properly selecting the smallest thumbnail greater than the provided threshold. This has been corrected. Note that this bug had an impact on page performance as unnecessarily large images were loaded on the page. (#471) - The `relativePath` computed by Jambo is now properly passed along to the Theme's cards. (#499)

…ade (#913) This folder is not needed by hitchhikers, and will slightly slow down the build when jambo tries to register all files in themes/[theme] as partials. Previously, there was a bug introduced in #441 where an async method was not being awaited, causing the tests folder to not be deleted. J=SLAP-1452 TEST=manual test upgrading yanswers to this branch, and see that there's no test-site or tests folder

creotutar requested review from oshi97, tmeyer2115, nanhu95, alexisgrow and cea2aj October 28, 2020 19:42

oshi97 reviewed Oct 28, 2020

View reviewed changes

postupgrade/PostUpgradeHandler.js Outdated Show resolved Hide resolved

PR fixes: syntax, remove trim, var name change

06a4ad2

oshi97 approved these changes Oct 29, 2020

View reviewed changes

creotutar merged commit 49af0ce into develop Oct 29, 2020

creotutar deleted the hotfix/postupgrade-merge branch October 29, 2020 16:58

tmeyer2115 mentioned this pull request Dec 1, 2020

v1.17 #520

Merged

tmeyer2115 mentioned this pull request Dec 1, 2020

v1.17.0 #521

Merged

oshi97 mentioned this pull request Aug 10, 2021

fix tests not being removed on upgrade, also remove test-site on upgrade #913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

postupgrade: Fix script for submodules, merge correctly, keep .git #441

postupgrade: Fix script for submodules, merge correctly, keep .git #441

creotutar commented Oct 28, 2020

oshi97 left a comment

oshi97 Oct 28, 2020

creotutar Oct 29, 2020

oshi97 Oct 29, 2020

oshi97 Oct 28, 2020

creotutar commented Oct 29, 2020

postupgrade: Fix script for submodules, merge correctly, keep .git #441

postupgrade: Fix script for submodules, merge correctly, keep .git #441

Conversation

creotutar commented Oct 28, 2020

oshi97 left a comment

Choose a reason for hiding this comment

oshi97 Oct 28, 2020

Choose a reason for hiding this comment

creotutar Oct 29, 2020

Choose a reason for hiding this comment

oshi97 Oct 29, 2020

Choose a reason for hiding this comment

oshi97 Oct 28, 2020

Choose a reason for hiding this comment

creotutar commented Oct 29, 2020